Skip to content
This repository was archived by the owner on Oct 25, 2021. It is now read-only.

Dmitriy_Vaschenko#47

Open
dvaschenko wants to merge 38 commits intomasterfrom
feature/DmitriyVaschenko
Open

Dmitriy_Vaschenko#47
dvaschenko wants to merge 38 commits intomasterfrom
feature/DmitriyVaschenko

Conversation

@dvaschenko
Copy link
Collaborator

No description provided.

@dvaschenko dvaschenko added the readyForReview Sign for Artem to take a look label Jul 12, 2021
@dvaschenko dvaschenko changed the title Feature/dmitriy vaschenko Dmitriy_Vaschenko Jul 12, 2021
@NikolaevArtem NikolaevArtem added bug Code fix needed and removed readyForReview Sign for Artem to take a look labels Jul 13, 2021
@dvaschenko dvaschenko added the readyForReview Sign for Artem to take a look label Jul 13, 2021
@NikolaevArtem NikolaevArtem removed the readyForReview Sign for Artem to take a look label Jul 15, 2021
@dvaschenko dvaschenko added the readyForReview Sign for Artem to take a look label Jul 18, 2021
@NikolaevArtem NikolaevArtem added HW_1 Good to go and removed bug Code fix needed labels Jul 19, 2021
import java.io.IOException;
import java.io.InputStreamReader;

public class PyramidPrinter {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Same for -1 and 5.4

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isValid method fixed

for (int i = 0; i < args.length; i++) {

String toPrint = args[i];
if (toPrint.equalsIgnoreCase("error")) { //checking for error
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: I think this part is self-explanatory

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

import java.util.Locale;

public class RandomCharsTable {
private final String ERR_MSG = "Passed parameters should match the format [positive integer] [positive integer] [even|odd]";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: could be static

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {

String rawInput = reader.readLine();
result = rawInput.split(" ");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result = rawInput.split(" ");
return rawInput.split(" ");

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in other places

Comment on lines 15 to 17
String lenStr;
String widStr;
String method;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: this way of initialization is rarely used in Java

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to single line init

@NikolaevArtem NikolaevArtem removed the readyForReview Sign for Artem to take a look label Sep 2, 2021
@dvaschenko dvaschenko added the readyForReview Sign for Artem to take a look label Sep 5, 2021
import course_project.Utils.ConsoleHandling.Printer;
import course_project.components.Field;

public class Main {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
The computer shot it shown only on the next computer turn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: cells around a destroyed ship could be marked as miss automatically
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-done! The game is confortable to play, has nice interface, automatic ship placement, informative messages. The visual representation is also good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the code perspective also looks good, there are minor issues but in general, the code is readable, easy to follow, has nice abstractions. You show good knowledge of Java Core and Java libraries. Approved!

Comment on lines +44 to +47
public void printYouWasHit() {
String YOU_WAS_HIT = "Uh, oh, your ship is falling apart!";
System.out.println(YOU_WAS_HIT);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: methdos like that are not needed, instead you should introduce static constants and just call sout(constant) where needed

import java.util.ArrayList;
import java.util.List;

public interface Placer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: it has a lot of logic, so it's more like an abstract class

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although you can use default in interfaces, it's not recommended

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants